DEV - Add Fontawesome scripts to build assets #1982
Conversation
trallard
added
tag: dependencies
Coverage report
This PR does not seem to contain any modification to coverable code. |
|
I can explain your We are dealing with a dependency failure between two scripts:
The Font Awesome library needs to be loaded first because the custom icon script calls it like so: FontAwesome.library.add(/* icon object */)The Font Awesome and custom icon scripts are loaded with different Jinja macros at different points in the layout template (which is the base template for all of the pages in the docs). Going from the top to the bottom of the template, here is a list of the relevant macros:
So this is the problem. The custom icon script depends on the Font Awesome script. Before PR 1955, the Font Awesome script was loaded before the custom icon script. Now it's loaded after. So when the custom icon script executes, it references an undefined global, There are a number of different ways we could try to solve this, but the most straightforward is to restore the Note that PR 1955 also added the |
|
Thanks @gabalafou to fix the immediate issues with FA I added the FontAwesome JS scripts to the |
|
Will also mark this as a release blocker as we need to get this merged to ensure that FA is properly vendored for end-users |
trallard
added
the
impact: block-release
trallard
changed the title
|
|
||
| import "@fortawesome/fontawesome-free/js/fontawesome.min.js"; |
There was a problem hiding this comment.
Why is this import needed?
Testing locally, if I remove fontawesome.min.js (but leave all.min.js) everything looks right on the home page. If I remove all.min.js (but leave fontawesom.min.js) the home page is missing icons.
Seems like only all.min.js needs to be imported here?
There was a problem hiding this comment.
I followed the docs at https://docs.fontawesome.com/web/setup/host-yourself/svg-js (this is also what we had before).
I think because fontawesome.js is the loader, we need that to allow for custom icons to be added through FontAwesome.library.add() all.min.js should only be the SVG icons (need to check, unless it is a catch all for utilities + icons). But it seems to work without it because we are getting the core utilities and icons via the css/scss version of FA anyway.
This is why I am considering getting rid of the scss/css vendors so we can only deal with one at a time; otherwise, it is really confusing to debug and have a clean picture of the dependencies. I can remove it at is seems to work but I am not happy with having both the scss/css and JS versions of FA right now.e
There was a problem hiding this comment.
Hmm. My knowledge of Font Awesome is shaky but I think the approach we're taking now is some kind of hybrid of the package manager and svg-js approaches.
There is a Font Awesome Python/Django docs page that says:
We have also provided a shortcut all.min.js or all.min.css file that includes all the Free icons as well as the core utility functions and styles. It’s a handy shortcut but isn’t as performant as selecting individual styles.
My understanding is that because we advertise PST as "Font Awesome Free included," we have to:
- Include all three of the free icon sets (regular, solid, and brand)
- Include both the svg+js and web fonts+css methods of using those icons, which means we have to ship both all.js and all.css down the wire to the end user.
There was a problem hiding this comment.
Hmm. My knowledge of Font Awesome is shaky but I think the approach we're taking now is some kind of hybrid of the package manager and svg-js approaches.
This is correct. We are doing some things from one and some from the other. This means we are vendoring both the JS and CSS FA variants (which have performance and maintenance overheads).
Include both the svg+js and web fonts+css methods of using those icons, which means we have to ship both all.js and all.css down the wire to the end user.
That is incorrect. We only need one or the other from the Python docs you linked to:
We have also provided a shortcut all.min.js OR all.min.css file that includes all the Free icons as well as the core utility functions and styles.
The or is the crucial part; regardless of how we vendor FA, the CSS or JS approach provides the same functionalities and end-user experience (syntax and use of the icons are the same functionally).
We only need to chose one and stick to it (of course this would mean some refactor in the codebase) but ultimately users of PST should have the same experience of "FA included".
|
|
||
| import "@fortawesome/fontawesome-free/js/fontawesome.min.js"; |
There was a problem hiding this comment.
Hmm. My knowledge of Font Awesome is shaky but I think the approach we're taking now is some kind of hybrid of the package manager and svg-js approaches.
There is a Font Awesome Python/Django docs page that says:
We have also provided a shortcut all.min.js or all.min.css file that includes all the Free icons as well as the core utility functions and styles. It’s a handy shortcut but isn’t as performant as selecting individual styles.
My understanding is that because we advertise PST as "Font Awesome Free included," we have to:
- Include all three of the free icon sets (regular, solid, and brand)
- Include both the svg+js and web fonts+css methods of using those icons, which means we have to ship both all.js and all.css down the wire to the end user.
|
Since I pushed changes to the function names, I need another review by @gabalafou. |
There was a problem hiding this comment.
Code changes look fine.
It looks like all of the icons load on the three following preview URLs:
And I did not see any related errors in the browser console or network log. Thus, I approve. Thanks for getting this through @trallard!
I seem to have forgotten to add the FA scripts to the `pyproject.toml` so I am adding it here (which should fix the FA rendering issue in the dev version of the docs). --------- Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>
I seem to have forgotten to add the FA scripts to the
pyproject.tomlso I am adding it here (which should fix the FA rendering issue in the dev version of the docs).@gabalafou Additionally, there seems to be a new issue:
Uncaught ReferenceError: FontAwesome is not defined at...where we add the custom PyData and PyPI icons. However, I do not know how to resolve this (this is beyond my JS knowledge).There is this document on customizing https://github.com/FortAwesome/Font-Awesome/wiki/Customize-Font-Awesome, which might be helpful for you. Could you please have a look?